Fix token_expiry_time upon context initialization for stored tokens.#1784
Fix token_expiry_time upon context initialization for stored tokens.#1784keurcien wants to merge 6 commits intomodelcontextprotocol:mainfrom
token_expiry_time upon context initialization for stored tokens.#1784Conversation
|
I'm having a similar issue so I am happy to see this PR. However, one concern is that OAuth 2.0 doesnt require the response to contain an expires_in.. In that case, if a 401 is returned but we do have refresh token available, should we try that first rather than doing the whole client flow? https://github.com/modelcontextprotocol/python-sdk/blob/main/src/mcp/client/auth/oauth2.py#L505 |
|
Hey @keurcien — I ran into the same bug against HubSpot's MCP Auth App and opened #2492 which fixes the related Your 4-line fix + tests here look clean and the CI is green apart from one flaky Windows run. If you're swamped and want me to rebase it onto current |
Hi @mangibaud33! Yes I don't have much time now so yeah please go ahead :) PS: I also use HubSpot's official MCP server, so I'm looking forward to using your fix (had to subclass the current token storage in my custom implementation). |
Partial fix for #1318.
Motivation and Context
When storing OAuth tokens in a persistent storage, the OAuthClientProvider context would never refresh tokens, because it would consider tokens valid (by
is_token_valid()) and proceed with the expiredaccess_tokenuntil it meets a 401 response from the MCP server, forcing users to start a new OAuth flow.The proposed change is small and only solve part of the problem. It's mainly inspired by the discussion from #1318 and the approach taken in the FastMCP library: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/client/auth/oauth.py
This fix works in the case where
token.expires_inreturned by theTokenStorageis well calculated (which is not the case if we just simply store theOAuthTokenas is, cf. below), and when the token endpoint is the defaultMCP_SERVER_URL/token.This PR could also be considered a draft, as it raises other questions about how
client_metadata(in case where token endpoint is not obvious) and how or wheretoken.expires_atvalues should be stored.How Has This Been Tested?
Here's a sample script to test the proposed change (tested with Notion MCP and Linear MCP). It uses a StoredToken class to hold an extra
expires_atvalue, to return a correcttoken.expires_invalue inget_tokens(cf. PrefectHQ/fastmcp@f73b7b5)Steps:
Breaking Changes
No breaking changes.
Types of changes
Checklist
Additional context
The added tests complement the existing
test_token_validity_checkas it assumes thetoken_expiry_timegets set at initialization.